Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(AsyncPipe): allow onError argument #7990

Closed
wants to merge 1 commit into from

Conversation

PatrickJS
Copy link
Member

@robwormald
Copy link
Contributor

what's the usage look like? something like

<div *ngFor="#foo of foos | async:onError">

?

@PatrickJS
Copy link
Member Author

@robwormald ya

@mhevery
Copy link
Contributor

mhevery commented Apr 17, 2016

Hey @robwormald & @gdi2290,

It seems a bit strange to me that one would put error handling in the UI.

  1. What is the current behavior in case of an error?
  2. What is the motivation for this? What would one do with this error other the report it?
  3. Could we think of a way to do this in a way which is more component scoped? As it a component could say 'I would like to get all async pipe errors delivered here'

PS: Just trying to get a better feel for the motivation behind thes PR.

@mhevery mhevery self-assigned this Apr 17, 2016
@mhevery mhevery closed this in 390046d May 19, 2016
@PatrickJS PatrickJS deleted the async-pipe-on-error branch May 20, 2016 00:59
vicb added a commit to vicb/angular that referenced this pull request May 26, 2016
This reverts commit 390046d.
CI fails for IE on win8.
PR angular#7990
KiaraGrouwstra pushed a commit to KiaraGrouwstra/angular that referenced this pull request Jun 10, 2016
KiaraGrouwstra pushed a commit to KiaraGrouwstra/angular that referenced this pull request Jun 21, 2016
This reverts commit 390046d.
CI fails for IE on win8.
PR angular#7990
@Martin-Luft
Copy link

Any plans to fix this PR?

@@ -1,4 +1,5 @@
import {isBlank, isPresent, isPromise, CONST} from 'angular2/src/facade/lang';
import {ListWrapper} from 'angular2/src/facade/collection';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this anymore

@stevenjob
Copy link

stevenjob commented Mar 20, 2017

Why was this closed?
The async pipe seems useless without a way to handle errors

@DzmitryShylovich
Copy link
Contributor

@stevenjob Observable has .catch() method

@stevenjob
Copy link

stevenjob commented Mar 20, 2017

doesn't catch require you to return an observable?
In reality you would want to show an error message or something?

An equivalent to .subscribe(_, handleError) is needed?

@DzmitryShylovich
Copy link
Contributor

DzmitryShylovich commented Mar 20, 2017

You can use if/else style syntax:

<ng-template #error>Error!</ng-template>
<div *ngIf="userObservable | async as user; else error">
  {{ user.name }}
</div>

// inside component
userObservable = this.http.get('user').catch(error => Observable.of(false));

(it's only available in ng4+)

@stevenjob
Copy link

Returning an observable of false from a stream that is supposed to return user would break typescript and seems like a smell to me.
Also i imagine the else here would be used to check for loading when the stream has not returned a value yet and would be falsy?

I have seen people use -1 for error value but its not a nice solution.

// inside component would get `Type Observable<bool> is not assignable to Observable<User> ...`
userObservable: Observable<User> = this.http.get('user').catch(error => Observable.of(false));

Since the async pipe is doing the same job as using subscribe it should handle all the same things as subscribe e.g. onNext, onError, onCompleted

To handle all cases:


<ng-template #loading>Loading!</ng-template>
<div *ngIf="userObservable | async:onError:onCompleted as user; else loading">
  {{ user.name }}
</div>

// inside component
userObservable = this.http.get('user');


public onError(error) {
  // do something with error
}

public onCompleted() {
  // do something
}

@DzmitryShylovich
Copy link
Contributor

Returning an observable of false from a stream that is supposed to return user would break typescript and seems like a smell to me.

ok, you can return null/undefined

Since the async pipe is doing the same job as using subscribe it should handle all the same things as subscribe e.g. onNext, onError, onCompleted

if you need all 3 callbacks just subscribe inside a component

@lordscales91
Copy link

So the conclusion is: never use async pipes if you want to handle errors properly, specially if you want to display an error message sent from the server.

@rgant
Copy link
Contributor

rgant commented Dec 4, 2018

A problem with AsyncPipe throwing errors is that it makes them uncatchable in jasmine tests as far as I can tell. If you are testing the error handling then you want the errors to make it to the template, but you don't want those errors to reach jasmine and cause the test to fail with Uncaught error. I had to work around this issue by patch AsyncPipe to not throw errors in my tests.

https://stackoverflow.com/q/52433159/41908

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(AsyncPipe): allow for error and/or complete callbacks as arguments to |async pipe
9 participants